Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-6300][Spark Core] sc.addFile(path) does not support the relative path. #4993

Closed
wants to merge 9 commits into from
Closed

Conversation

pzzs
Copy link
Contributor

@pzzs pzzs commented Mar 12, 2015

when i run cmd like that sc.addFile("../test.txt"), it did not work and throwed an exception:
java.lang.IllegalArgumentException: java.net.URISyntaxException: Relative path in absolute URI: file:../test.txt
at org.apache.hadoop.fs.Path.initialize(Path.java:206)
at org.apache.hadoop.fs.Path.(Path.java:172)
........
.......
Caused by: java.net.URISyntaxException: Relative path in absolute URI: file:../test.txt
at java.net.URI.checkPath(URI.java:1804)
at java.net.URI.(URI.java:752)
at org.apache.hadoop.fs.Path.initialize(Path.java:203)

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@sryza
Copy link
Contributor

sryza commented Mar 12, 2015

Thanks for taking this up @DoingDone9. Would it be possible to add a test for this?

val schemeCorrectedPath = uri.getScheme match {
case null | "local" => "file:" + uri.getPath
case null | "local" => "file:" + file.getCanonicalPath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For scoping, probably better to move new File(path) into the match block.

Also, we should include the "file" scheme in the case as well.

@pzzs
Copy link
Contributor Author

pzzs commented Mar 12, 2015

I have added the test, please test it. @sryza

@srowen
Copy link
Member

srowen commented Mar 12, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Mar 12, 2015

Test build #28516 has started for PR 4993 at commit 0ff3fa8.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 12, 2015

Test build #28516 has finished for PR 4993 at commit 0ff3fa8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28516/
Test PASSed.

@@ -1093,7 +1093,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
def addFile(path: String, recursive: Boolean): Unit = {
val uri = new URI(path)
val schemeCorrectedPath = uri.getScheme match {
case null | "local" => "file:" + uri.getPath
case null | "local" => "file:" + new File(path).getCanonicalPath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be slightly more robust as new File(path).getCanonicalFile.toURI.toString. Then the "file:" can go away too.

@SparkQA
Copy link

SparkQA commented Mar 13, 2015

Test build #28546 has started for PR 4993 at commit d594e16.

  • This patch merges cleanly.

@pzzs
Copy link
Contributor Author

pzzs commented Mar 13, 2015

I have changed it. Please test it. @srowen

@SparkQA
Copy link

SparkQA commented Mar 13, 2015

Test build #28547 has started for PR 4993 at commit ee375cd.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 13, 2015

Test build #28546 has finished for PR 4993 at commit d594e16.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28546/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Mar 13, 2015

Test build #28547 has finished for PR 4993 at commit ee375cd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28547/
Test PASSed.

@pzzs
Copy link
Contributor Author

pzzs commented Mar 13, 2015

/cc @sryza @srowen

test("addFile works") {
val file = File.createTempFile("someprefix", "somesuffix")
val absolutePath = file.getAbsolutePath
val file1 = File.createTempFile("someprefix1", "somesuffix1")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't use the temp dir you created below.

if (!gotten1.exists()) {
throw new SparkException("file doesn't exist : " + absolutePath1)
}
if (!gotten2.exists()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same; feels like you can wrap up all these checks in a local function that you can just apply to both files rather than repeating code.

@srowen
Copy link
Member

srowen commented Mar 16, 2015

I think this is OK to go actually. The refactoring of the common code isn't a big deal, and, the temp dir issue is something I'm addressing in bulk in SPARK-6338 right now anyway, so I could touch it up along with that logical change.

asfgit pushed a commit that referenced this pull request Mar 16, 2015
…ve path.

when i run cmd like that sc.addFile("../test.txt"), it did not work and throwed an exception:
java.lang.IllegalArgumentException: java.net.URISyntaxException: Relative path in absolute URI: file:../test.txt
at org.apache.hadoop.fs.Path.initialize(Path.java:206)
at org.apache.hadoop.fs.Path.<init>(Path.java:172)
........
.......
Caused by: java.net.URISyntaxException: Relative path in absolute URI: file:../test.txt
at java.net.URI.checkPath(URI.java:1804)
at java.net.URI.<init>(URI.java:752)
at org.apache.hadoop.fs.Path.initialize(Path.java:203)

Author: DoingDone9 <799203320@qq.com>

Closes #4993 from DoingDone9/relativePath and squashes the following commits:

ee375cd [DoingDone9] Update SparkContextSuite.scala
d594e16 [DoingDone9] Update SparkContext.scala
0ff3fa8 [DoingDone9] test for add file
dced8eb [DoingDone9] Update SparkContext.scala
e4a13fe [DoingDone9] getCanonicalPath
161cae3 [DoingDone9] Merge pull request #4 from apache/master
c87e8b6 [DoingDone9] Merge pull request #3 from apache/master
cb1852d [DoingDone9] Merge pull request #2 from apache/master
c3f046f [DoingDone9] Merge pull request #1 from apache/master

(cherry picked from commit 00e730b)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@asfgit asfgit closed this in 00e730b Mar 16, 2015
vanzin pushed a commit to vanzin/spark that referenced this pull request Apr 20, 2015
…ve path.

when i run cmd like that sc.addFile("../test.txt"), it did not work and throwed an exception:
java.lang.IllegalArgumentException: java.net.URISyntaxException: Relative path in absolute URI: file:../test.txt
at org.apache.hadoop.fs.Path.initialize(Path.java:206)
at org.apache.hadoop.fs.Path.<init>(Path.java:172)
........
.......
Caused by: java.net.URISyntaxException: Relative path in absolute URI: file:../test.txt
at java.net.URI.checkPath(URI.java:1804)
at java.net.URI.<init>(URI.java:752)
at org.apache.hadoop.fs.Path.initialize(Path.java:203)

Author: DoingDone9 <799203320@qq.com>

Closes apache#4993 from DoingDone9/relativePath and squashes the following commits:

ee375cd [DoingDone9] Update SparkContextSuite.scala
d594e16 [DoingDone9] Update SparkContext.scala
0ff3fa8 [DoingDone9] test for add file
dced8eb [DoingDone9] Update SparkContext.scala
e4a13fe [DoingDone9] getCanonicalPath
161cae3 [DoingDone9] Merge pull request #4 from apache/master
c87e8b6 [DoingDone9] Merge pull request #3 from apache/master
cb1852d [DoingDone9] Merge pull request #2 from apache/master
c3f046f [DoingDone9] Merge pull request #1 from apache/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants